Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

service/header/core_listener: Fix bug where old block headers are spammed to headersub by bridge nodes #507

Closed
wants to merge 5 commits into from
Closed

service/header/core_listener: Fix bug where old block headers are spammed to headersub by bridge nodes #507

wants to merge 5 commits into from

Conversation

renaynay
Copy link
Member

@renaynay renaynay commented Mar 9, 2022 •
edited
Loading

Resolves #499.

@renaynay renaynay added area:header Extended header area:core_and_app Relationship with Core node and Celestia-App labels Mar 9, 2022
@adlerjohn
Copy link
Member

If there are still TODOs, can we convert this to draft?

@renaynay
Copy link
Member Author

renaynay commented Mar 9, 2022

How do you turn it back to draft mode @adlerjohn ?

@adlerjohn
Copy link
Member

draft

@renaynay renaynay marked this pull request as draft March 9, 2022 14:50
@renaynay renaynay marked this pull request as ready for review March 9, 2022 15:51
@renaynay renaynay marked this pull request as draft March 9, 2022 15:51
@renaynay renaynay marked this pull request as ready for review March 10, 2022 11:30
@renaynay
Copy link
Member Author

TestSyncStartStopLightWithBridge is flakey on main branch as well.

adlerjohn
adlerjohn reviewed Mar 12, 2022
View reviewed changes
core/fetcher.go Outdated Show resolved Hide resolved
core/fetcher.go Outdated Show resolved Hide resolved
core/fetcher.go Outdated Show resolved Hide resolved
@renaynay
docs|feature: addressed Johns comments, added docs and introduced tim…
9c2e17e 
…e.Sleep for second before requesting sync state again
Wondertan
Wondertan reviewed Mar 21, 2022
View reviewed changes
service/header/core_listener.go Outdated Show resolved Hide resolved
service/header/core_listener.go Outdated Show resolved Hide resolved
service/header/core_listener.go Outdated Show resolved Hide resolved
core/fetcher.go Outdated Show resolved Hide resolved
@Wondertan
Copy link
Member

Appendix to my review. The assumption about fixing the issue has to be verified somehow. Manual verification is also fine and shouldn't be hard to setup

@renaynay renaynay mentioned this pull request Mar 21, 2022
Closed
@renaynay
Copy link
Member Author

Core_listener should still listen for new blocks while bridge node syncing and erasure code them but should not broadcast them yet.

liamsi
liamsi approved these changes Mar 25, 2022
View reviewed changes
Copy link
Member

@liamsi liamsi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a straightforward change and PR. Thanks!

liamsi
liamsi reviewed Mar 25, 2022
View reviewed changes
Copy link
Member

@liamsi liamsi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Only thing that could be improved is related to:

Core_listener should still listen for new blocks while bridge node syncing and erasure code them but should not broadcast them yet.

Ideally this was captured by a test somehow (unit/swamp).

@Wondertan
Copy link
Member

We found an issue with the approach there and after discussing plenty of solutions decided to change the solution here. Specifically, we decided to contribute to the PubSub for a specific feature that would allow us to solve the problem: libp2p/go-libp2p-pubsub#481
We won't wait for it to be accepted as we maintain our own PubSub fork due to #306, so I am cherry-picking the solution now and making an alternative PR. If something is change on main pubsub branch, I will do changes here acrrodingly.

@Wondertan Wondertan mentioned this pull request Mar 25, 2022
Merged
@Wondertan
Copy link
Member

Closed in favour of #549

@Wondertan Wondertan closed this Mar 25, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Wondertan Wondertan Wondertan left review comments

@liamsi liamsi liamsi approved these changes

@adlerjohn adlerjohn adlerjohn approved these changes

Assignees

@renaynay renaynay

Labels
area:core_and_app Relationship with Core node and Celestia-App area:header Extended header
Projects
No open projects
Celestia Node
Archived in project
Milestone
No milestone
4 participants
@renaynay @adlerjohn @Wondertan @liamsi